Skip to content

fix(cam_hal): guard cam_verify_jpeg_eoi() against buffer-underflow #759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 14, 2025
Merged

fix(cam_hal): guard cam_verify_jpeg_eoi() against buffer-underflow #759

merged 1 commit into from
Jul 14, 2025
+7 −2

Conversation

RubenKelevra
Copy link
Contributor

@RubenKelevra RubenKelevra commented Jul 1, 2025 •
edited
Loading

Description

If DMA returns a frame shorter than two bytes, the previous code did:

dptr = inbuf + length - 2;

which under-flows the pointer and produces undefined behaviour.

Behaviour for valid frames (length ≥ 2) is unchanged; damaged or empty buffers are now discarded safely.

Related

In the search for the cause of crashes reported by @turenkomv here: esphome/esphome#8832 (comment) I found this underflow of a pointer, which can cause undefined behaviour.

Testing

@turenkomv would you be so kind and would test this on your hardware? Tag is fixes_for_m5_0.2, which is the currently used version in ESPHome with both patches on top.

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass. (?)
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@RubenKelevra RubenKelevra mentioned this pull request Jul 1, 2025
Merged
6 tasks
@turenkomv
Copy link

No effect, still getting the same error.

Just to double-check — am I only testing the changes from this PR, and not these ones #758 ?

@RubenKelevra
Copy link
Contributor Author

Just to double-check — am I only testing the changes from this PR, and not these ones #758 ?

No, please both changes. The tag fixes_for_m5_0.2 I've prepared for your contains both fixes on top of 2.0.15, which is the version used in ESPHome.

@turenkomv
Copy link

Tried fixes_for_m5_0.2 — no changes, same behavior

@RubenKelevra
Copy link
Contributor Author

Yeah, I found the next bug.

Grab a coffee, this may take a while :D

@turenkomv
Copy link

Enabled the flags:

CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK: "y"
CONFIG_FREERTOS_CHECK_STACKOVERFLOW: "2"

Here's the output from xtensa-esp32s3-elf-addr2line — hopefully this helps:

C:\Projects\xtensa-esp32s3-elf-gcc8_4_0-esp-2021r2-win32\xtensa-esp32s3-elf\bin>xtensa-esp32s3-elf-addr2line.exe -pfiaC -e "C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1\.pioenvs\m5stack-camera-1\firmware.elf" 0x4037ef47 0x420484af 0x420485d5 0x4200f951 0x42016a3e 0x420a4a49 0x403845b2 0x4201a72a 0x42019cca 0x4200bbfa
0x4037ef47: xPortSetInterruptMaskFromISR at C:\Users\turen\.platformio\packages\framework-espidf\components\freertos\FreeRTOS-Kernel\portable\xtensa\include\freertos/portmacro.h:552
 (inlined by) xPortEnterCriticalTimeout at C:\Users\turen\.platformio\packages\framework-espidf\components\freertos\FreeRTOS-Kernel\portable\xtensa/port.c:478
0x420484af: vPortEnterCritical at C:\Users\turen\.platformio\packages\framework-espidf\components\freertos\FreeRTOS-Kernel\portable\xtensa\include\freertos/portmacro.h:567
 (inlined by) find_key at C:\Users\turen\.platformio\packages\framework-espidf\components\pthread/pthread_local_storage.c:77
0x420485d5: pthread_setspecific at C:\Users\turen\.platformio\packages\framework-espidf\components\pthread/pthread_local_storage.c:199
0x4200f951: esphome::logger::Logger::check_and_set_task_log_recursion_(bool) at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/src/esphome/components/logger/logger.h:298
 (inlined by) esphome::logger::Logger::check_and_set_task_log_recursion_(bool) at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/src/esphome/components/logger/logger.h:287
 (inlined by) esphome::logger::Logger::log_vprintf_(unsigned char, char const*, int, char const*, __va_list_tag) at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/src/esphome/components/logger/logger.cpp:35
0x42016a3e: esphome::esp_idf_log_vprintf_(char const*, __va_list_tag) at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/src/esphome/core/log.cpp:56
0x420a4a49: esp_log_writev at C:\Users\turen\.platformio\packages\framework-espidf\components\log/log.c:210
0x403845b2: esp_log_write at C:\Users\turen\.platformio\packages\framework-espidf\components\log/log.c:220
0x4201a72a: cam_take at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/managed_components/espressif__esp32-camera/driver/cam_hal.c:494 (discriminator 1)
0x42019cca: esp_camera_fb_get at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/managed_components/espressif__esp32-camera/driver/esp_camera.c:372
0x4200bbfa: esphome::esp32_camera::ESP32Camera::framebuffer_task(void*) at C:\Projects\m5stack-camera-1\.esphome\build\m5stack-camera-1/src/esphome/components/esp32_camera/esp32_camera.cpp:412

@RubenKelevra
Copy link
Contributor Author

Yes, that's very helpful.

@RubenKelevra
Copy link
Contributor Author

I found the culprit. I'll provide another fix.

This was referenced Jul 1, 2025
Closed
Merged
@me-no-dev
Copy link
Member

Do not use sizeof here and please let me know in whta order your PRs need to go in. they seem all mixed up

@RubenKelevra RubenKelevra marked this pull request as draft July 14, 2025 10:33
@RubenKelevra
Copy link
Contributor Author

Hey @me-no-dev,

I'm sorry for the confusion: So #758 (which was already merged), this one, and #760 are completely independent from each other.

#765 however requires all 3 independent PRs to be merged, that's why it contained the commits of the unmerged PRs. I've removed the already merged PR #758 from it.

@RubenKelevra
fix(cam_hal): guard cam_verify_jpeg_eoi() against buffer-underflow
ee92090 
If DMA returns a frame shorter than two bytes, the previous code
did:

    dptr = inbuf + length - 2;

which under-flows the pointer and produces undefined behaviour.

Behaviour for valid frames (length ≥ 2) is unchanged; damaged or
empty buffers are now discarded safely.
@RubenKelevra RubenKelevra force-pushed the fix_jpeg-eoi-underflow branch from db1989a to ee92090 Compare July 14, 2025 11:21
@me-no-dev
Copy link
Member

ready for review and merge?

@RubenKelevra
Copy link
Contributor Author

Do not use sizeof here

Fixed. Ready for merge.

@RubenKelevra RubenKelevra marked this pull request as ready for review July 14, 2025 11:55
@me-no-dev me-no-dev merged commit a42fe29 into espressif:master Jul 14, 2025
30 checks passed
@RubenKelevra RubenKelevra deleted the fix_jpeg-eoi-underflow branch July 15, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RubenKelevra @turenkomv @me-no-dev